Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add reportActiveConfigurationProfile command #9325

Merged
merged 4 commits into from Apr 14, 2020

Conversation

feerrenrut
Copy link
Contributor

@feerrenrut feerrenrut commented Feb 25, 2019

Link to issue number:

Initially developed due to #9239 (comment)

Summary of the issue:

Even though the issue of announcing the current profile was resolved in #9239 reporting the current profile name may be useful more generally.

Description of how this pull request fixes the issue:

A new command is introduced that allows the active configuration profile to be reported.
Reports "normal configuration profile active" or "{profileName} configuration profile active"
No default gesture is assigned.

Testing performed:

Checked that the correct name is reported when a configuration profile is active, and when the normal configuration is active.

Known issues with pull request:

Change log entry:

New Features
- Added a command to report the active configuration profile (#9325).

@DrSooom
Copy link

DrSooom commented Feb 26, 2019

def script_reportActiveConfigurationProfile(self, gesture):

"(self, gesture):" » "(self,gesture):" (See a few lines below)

What global input gesture is used for this command? NVDA+CTRL+Shift+P would be possible. And I guess that the message output will be via speech and braille together, right?

@LeonarddeR
Copy link
Collaborator

You could consider assigning shift+NVDA+p as default.

@feerrenrut
Copy link
Contributor Author

"(self, gesture):" » "(self,gesture):" (See a few lines below)

Visually, code without spaces is harder to read. So while much of the code is formatted this way, though certainly not all, I hope we are able to start to transition.

And I guess that the message output will be via speech and braille together, right?

Correct, although I have not tested with a braille display.

You could consider assigning shift+NVDA+p as default.

Given the revised approach to #9325 (not changing the title text), this command might not be so urgent. If there is demand for it, we could still consider including it. However I think we should be cautious about assigning a default gesture.

@DrSooom
Copy link

DrSooom commented Feb 27, 2019

@feerrenrut: Not only visually, also on a braille display. As I'm not familiar with Python, I didn't know that both methods are valid and equal. I thought it was a typo. Thanks for the explanation.

@JulienCochuyt
Copy link
Collaborator

You could consider assigning shift+NVDA+p as default.

As @DrSooom proposed in #9325 (comment) I think NVDA+control+shift+p is more relevant as it would:

Btw, I am not at all in favor of unassigned default gestures for commands that can be useful for beginners to intermediate users.

@feerrenrut
Copy link
Contributor Author

The issue this PR attempted to address was resolved in the PR. However I think it could still be useful functionality, so I'll mark it as ready for review. Since sensible gestures are short supply I'd prefer to leave this unbound initially. I don't think that working with profiles is a beginner feature.

@feerrenrut feerrenrut marked this pull request as ready for review April 6, 2020 16:40
source/globalCommands.py Outdated Show resolved Hide resolved
@LeonarddeR
Copy link
Collaborator

@lukaszgo1 has a good point.

@AppVeyorBot

This comment has been minimized.

@feerrenrut
Copy link
Contributor Author

I have addressed the changes. The core concept was already reviewed. I'm going to merge this PR.

@feerrenrut feerrenrut merged commit da6d460 into master Apr 14, 2020
@feerrenrut feerrenrut deleted the addReportCurrentConfigurationProfile branch April 14, 2020 11:43
@nvaccessAuto nvaccessAuto added this to the 2020.1 milestone Apr 14, 2020
@feerrenrut feerrenrut modified the milestones: 2020.1, 2020.2 Apr 14, 2020
feerrenrut added a commit that referenced this pull request Apr 14, 2020
@Adriani90
Copy link
Collaborator

is this documented accordingly in the user guide?

@feerrenrut
Copy link
Contributor Author

@Adriani90 Are all commands documented in the user guide? It's not a particularly interesting command in need of explanation, and is listed in the gestures dialog.

@Adriani90
Copy link
Collaborator

Adriani90 commented Apr 16, 2020 via email

@feerrenrut
Copy link
Contributor Author

feerrenrut commented Apr 16, 2020

Then we have two places to maintain the list of commands, and two places to keep translations consistent. I don't think adding something to the user guide is a good way to make a new feature discoverable, it's main purpose is to explain confusing or complicated options. The changes file was updated, its purpose is discoverability. Otherwise, for a listing of all commands users should consult the inputGestures dialog. To specifically make unassigned commands more discoverable, we should enhance the input gestures dialog with filters.

@Adriani90
Copy link
Collaborator

@feerrenrut I agree. If there is a filter implemented, then it would help to have a cathegory which displays only the unassigned gestures for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants